-
Notifications
You must be signed in to change notification settings - Fork 170
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix false positive use_decorated_box
and use_colored_box
#3271
base: main
Are you sure you want to change the base?
Fix false positive use_decorated_box
and use_colored_box
#3271
Conversation
// Not lint if decoration is null or nullable because a DecoratedBox | ||
// only accepts a non-null Decoration | ||
if (DartTypeUtilities.isNonNullable( | ||
context, argument.expression.staticType)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking for nullability is good, but we're missing the more important test to ensure that the type of the expression is assignable to BoxDecoration
.
If you wouldn't mind fixing that while you're at it, then also add a test to ensure that we don't lint when the decoration
argument is some other kind of decoration (maybe a ShapeDecoration
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A Container
accepts a Decoration?
, which looks like a deliberate design decision to me. This also doesn't throw an error:
child: Container(decoration: const ShapeDecoration(shape: CircleBorder()))
If a Container is supposed to only accept a BoxDecoration
, why not just allow the constructor to accept a BoxDecoration
instead of a Decoration
?
class Color { | ||
Color(int value); | ||
} | ||
class Color {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't do so in this PR because the change has to be made to the analyzer package, but this class really ought to be part of the mock SDK we use for testing. When we do that we'll make the class match the published version of Color
, including making the constructor take an argument.
Once it's been added, then this test will need to be converted to using the definition from the mock SDK, and that will require restoring all of the arguments. It would ease that future conversion if you could revert the change to the definition of Color
and the invocations of the constructor.
@@ -20,7 +20,7 @@ Widget containerWithKey() { | |||
|
|||
Widget containerWithDecoration() { | |||
return Container( // LINT | |||
decoration: BoxDecoration(), | |||
decoration: Decoration(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that these changes (from BoxDecoration
to Decoration
) also need to be reverted. It's a false positive to lint here because this code can't use DecoratedBox
.
Sorry for the year+ delay, @minhqdao . @bwilkerson left some comments, are you able to review them and upload another commit? Thanks! |
This PR alters
use_decorated_box
so it doesn't advise switching from aContainer
to aDecoratedBox
when a nullableDecoration?
expression is used because aDecoratedBox
only accepts a non-nullDecoration
.The same is done for
use_colored_box
.See discussion: #3239